-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Fix spinner/Esc interrupt when MCP startup completes mid-turn #8661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
f75b188 to
07991af
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@2mawi2, thanks for the contribution. Sorry for the slow reply — we got behind over the holidays, but we're now back and going through the queue. Could you please fix the merge conflicts? |
|
@etraut-openai done and tested one more time! |
Fixes openai#7017 Signed-off-by: 2mawi2 <[email protected]>
ChatWidget treats the bottom-pane "task running" indicator as a derived UI-busy signal. The UI can be busy for two independent reasons: an agent turn (TurnStarted..complete/abort) and MCP startup (McpStartupUpdate..McpStartupComplete). Document the invariant that the bottom-pane indicator is driven by agent_turn_running || mcp_startup_status.is_some(), and that turn cleanup does not clear MCP startup tracking. Also add brief module headers to the ChatWidget test modules and clarify the MCP startup completion regression test intent.
a101896 to
be54e27
Compare
|
I rebased against main, and added a few docs to help clarify how this works. Thanks for the PR. Looks great |
Fixes #7017
What
Why
How